Repository navigation
Feat: add spinner with fade-in to charts (part 2/2) - #103210
luacmartins merged 10 commits into
Conversation
sharabai
changed the title
|
@Expensify/design Here’s what the fade-in looks like. What do you think? Screen.Recording.2026-10-08.at.17.58.15.mov |
That's feeling pretty good to me! |
|
Quick update: this PR got too big to review in one go, so I split it. #103684 is the first part, and it'll be merged first. It has the refactor that sizes each chart from its card's width before the chart mounts. I'll rebase this branch onto #103684 and force push, so the diff here only shows what's left:
|
57bff60 to
99d61e7
Compare
sharabai
changed the title
82ad5b5 to
4c0ac5a
Compare
sharabai
changed the title
|
@codex review |
|
@MelvinBot review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@MelvinBot find regressions |
ReviewI found no correctness bugs, and all CI checks pass except Suggestion: let touches pass through the spinner overlay during the fade. In What I checked
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 751a00e6f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // The chart this stands in for holds its own loading spinner at exactly this height. | ||
| const SPINNER_CARD_HEIGHT = CHART_CONTENT_MIN_HEIGHT; | ||
| // Home's first insight, spend over time, is a line chart, which holds its loading spinner at this height. | ||
| const SPINNER_CARD_HEIGHT = getCartesianChartHeight(); |
There was a problem hiding this comment.
When an Insights-beta user has persisted Top Spenders, Top Merchants, or Top Categories as the Home insight, Home restores that supporting chart rather than the first line chart; InsightsSectionContent.tsx:100 passes shouldShowGroupLabels={false}, so the bar/pie wrappers reserve only the 250px content height. This unconditional getCartesianChartHeight() reserves 289px instead, making the cold-start skeleton 39px taller than the restored card and causing it to visibly shrink; derive this height from the restored chart or otherwise accommodate the label-free selections.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The 39px shrink is real, but neither option fits this PR, so the skeleton stays matched to spend over time, the first insight and the one Home falls back to.
Deriving the height from the restored chart isn't possible: the skeleton only shows while the first OpenApp is pending on an empty store (useInFlightRequests.ts:196-199), which means right after sign-in or a cache reset, and sign-out doesn't keep the saved insight (SignInRedirect.ts:56-68). It comes back in the same OpenApp response that removes the skeleton, so while the skeleton is up there's nothing to tell it which chart is coming. Accommodating the label-free charts would mean one height for every Home insight, which changes how the loaded card looks, so it belongs with design rather than in this PR.
|
🤖 Web test steps verification results: ❌ fail The line chart rendered correctly the first time. After that, the Spend section, including the plain Expenses list, got stuck on its row-loading skeleton and never recovered, so the bar, pie, and reload checks couldn't run. I don't think this PR causes the stall: the Expenses list renders no chart, and this PR doesn't touch Search data loading. A code review of every chart consumer (Home Insights, the Insights page, and Spend) also found no regressions.
|
Not a bug, so no change: on native the overlay can't take a tap or hover from the chart in that window. It sits at opacity 0 for the whole window, set on the UI thread, and neither native touch path hit-tests a view that transparent: iOS skips anything under alpha 0.01 (RCTViewComponentView.mm:746), and gesture handler on Android, which carries every chart tap and hover, skips anything under 0.1 (GestureHandlerOrchestrator.kt:912). So on native the overlay already behaves as if it had |
Not caused by this PR: in each failed step Search never got its own content on screen, and the PR doesn't change any Search code. That includes the blank Top categories area. A chart view always renders inside a card with its title, and that card sits outside the chart code (SearchChartWrapper.tsx:22-27), so a chart that hadn't faded in yet would still leave a titled card behind. The screenshot has no card at all, which means Search never showed its chart view there. Nothing in the run's artifacts shows a rendered chart, though. The screenshot captioned as the rendered line chart shows the same row skeleton, and the recording ends while the test expenses are still being created, so the chart steps went untested rather than failing. |
Reviewer Checklist
Screenshots/VideosDetailstest.1.movtest.3.movtest.4.mov |
| <View> | ||
| <Animated.View style={fadeStyle}>{children}</Animated.View> | ||
| {!isRevealed && ( | ||
| <Animated.View style={[styles.chartActivityIndicator, StyleUtils.getHeight(loadingHeight), isReady && [styles.pAbsolute, styles.t0, styles.l0, styles.r0, spinnerStyle]]}> |
There was a problem hiding this comment.
The spinner overlay is position: absolute with no pointerEvents, and opacity: 0 disables neither hit-testing nor the accessibility tree - so between the fade starting on the UI thread and scheduleOnRN(setIsRevealed, true) landing on JS, it swallows every hover/tap over the chart and keeps shadowing the chart's accessible elements from VoiceOver.
Could you add pointerEvents="none" and hide the overlay from the a11y tree once it's transparent?
<Animated.View
pointerEvents="none"
accessibilityElementsHidden={isReady}
importantForAccessibility={isReady ? 'no-hide-descendants' : 'auto'}
style={[styles.chartActivityIndicator, StyleUtils.getHeight(loadingHeight), isReady && [styles.pAbsolute, styles.t0, styles.l0, styles.r0, spinnerStyle]]}
>There was a problem hiding this comment.
@daledah Thanks for flagging this! You're right that the window exists. That's actually why the spinner gets hidden on the UI thread instead of waiting for it to unmount. But I think opacity 0 may already handle both things you mentioned on iOS and Android.
I tested on the iOS simulator and an Android emulator, with the overlay forced to stay on top of a drawn chart:
- At opacity 0, tapping where the spinner sits opened the chart's tooltip, and the spinner wasn't in the accessibility tree.
- At opacity 1, the same tap didn't reach the chart, and the spinner was back in the tree.
I also turned on TalkBack on the emulator. It never stopped on the spinner, at either opacity, because the spinner has no label to read. I haven't tried VoiceOver or a real phone, though. Have you seen it act differently on a device?
Web is a bit different. There, opacity 0 doesn't block clicks or hide things from screen readers. But the overlay is usually gone before it turns see-through: it fades a frame after the chart starts showing, and React has removed it by then. Adding pointerEvents="none" would also let clicks through on web while the spinner is still showing over the hidden chart, so you could click into a chart you can't see yet. And the two accessibility props don't do anything on web.
So I'm leaning toward keeping it as is. Does that sound okay to you?
There was a problem hiding this comment.
After checking again, you’re right. I think it’s fine to leave it as is 👍
Screen.Recording.2026-10-22.at.15.28.41.mov@sharabai Hmm, it looks weird on my end. I don’t see a spinner when I test with Slow 4G throttling. |
|
@sharabai please resolve conflict |
@daledah Thanks for catching this, and sorry for the confusion! What you saw is expected, but that's on me: the test steps didn't make it clear enough that the reload matters. Each chart type shows the spinner only the first time it appears in the tab, while its code downloads, plus the chart engine for the very first chart. Without a reload, your tab already had all three chart types loaded, so each chart faded straight in with nothing left to wait for. Slow 4G mostly slows the Search request, and Search covers that with its own row skeleton. I've updated the PR description, and reloading on Spend is now its own step in test 2, so it's hard to miss. After the reload you should see the spinner for line, bar and pie. About the conflict: main is merged, but some chart heights changed there, so I'm still updating the code to match them. Please hold off until I tag you again. |
|
@daledah The chart heights now match main's new Home layouts, so you're good to continue. I've updated the PR description for them too, and test 3 now covers Home's bar and pie charts. |
|
checking again now |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 luacmartins has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.5.21-0 🚀
|
|
No help site update needed. This PR only changes how charts load: one spinner holds its place, then the chart fades in. No article describes chart loading, so no sentence is now wrong. |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.5.23-0 🚀
|
|
No help site update needed. This PR only changes how charts load, with one steady spinner followed by a fade-in, and no article describes chart loading or layout, so no sentence is now wrong. |
|
Deploy Blocker #105108 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.5.23-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Part 2 of 2 of the chart spinner fade-in, building on #103684, which sizes each chart from its card width.
Each chart entry point (
BarChart,LineChart,PieChart, web and native) now renders insideChartFrame, which putsChartRevealinsideChartWidthBox.ChartRevealkeeps one spinner up from the chart engine download until the chart body reports throughuseReportChartLoadingthat its data and fonts are ready, and the card has been measured. The chart then mounts at opacity 0 under the spinner, waitsHOLD_FRAMES(5) frames for Skia's first draw, and fades in. The frames are counted on the UI thread, where native Skia draws, because the JS thread can be busy for seconds after launch. A chart that mounts with everything in memory has no spinner to hold, so it fades in as soon as its card is measured, in place of the spinner #103684 showed there on native.The spinner box takes the loaded chart's height from
utils/chartHeights.ts, and Home's skeleton spinner card matches it. Home's Insights card wraps bar labels onto two lines and places the pie legend beside or under the donut (#104466, merged in here), soChartFramehands the measured card width togetLoadingHeight. Wrapped labels reserve both lines, and the pie picks its legend layout with the samehasRoomForSideLegendcheck the chart draws with.Important
A chart with angled x-axis labels, or a pie outside Home whose legend wraps past one row, makes its card grow as it mounts under the spinner. The spinner box reserves one line of flat labels and one legend row, because the label angle and the row count depend on the data. The spinner stays in place and the card grows below it.
On Home, a pie whose legend sits under the donut reserves five one-line rows, so the card shrinks with fewer categories and grows when a name wraps. Five is the most Home shows, and the count arrives with the data.
On native, a chart that loads at app start can leave its card blank for a moment between the spinner and the chart. On a cold start, native Skia puts its first frame on screen after the hold has ended and gives no signal to wait for, so the chart appears partway through its fade. Main removes its spinner as soon as the data is ready, so it shows the same blank with no hold covering any of it.
Fixed Issues
$ #103964
PROPOSAL:
Follow-up to #99560
Tests
Preconditions:
Spend over time.All five tests exercise the change.
1. Home keeps the chart's spinner in place until the chart fades in (Account A, web)
Spend over timechart fades in, and that the card does not change height when the chart appears.2. Each chart type fades in without the card jumping (Account A, web)
3. A chart picked on Home shows the spinner again and keeps the card height (Account A, web)
4. Without WebGL, the error message replaces the spinner (Account A, web)
open -a "Google Chrome" --args --disable-webgl5. On native, only a loading chart shows the spinner (Account A, iOS or Android)
Offline tests
N/A: this change only affects how a chart looks while it loads. It makes no requests and writes no Onyx data.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
pr-103210-test-5-native-only-loading-chart-shows-spinner.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
pr-103210-test-1-home-keeps-chart-spinner-in-place-until-fade-in.mp4
pr-103210-test-2-each-chart-type-fades-in-without-card-jumping.mp4
pr-103210-test-3-reloaded-chart-shows-spinner-again.mp4
pr-103210-test-4-without-webgl-error-replaces-spinner.mp4